-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: deep merge behavior in flat config #17906
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
}); | ||
|
||
it("overrides an object with an array", () => { | ||
const first = { foo: 42 }; | ||
const second = ["bar", "baz"]; | ||
const result = merge(first, second); | ||
|
||
assert.strictEqual(result, second); | ||
}); | ||
|
||
it("merges an array with an object", () => { | ||
const first = ["foo", "bar"]; | ||
const second = { baz: 42 }; | ||
const result = merge(first, second); | ||
|
||
assert.deepStrictEqual(result, { 0: "foo", 1: "bar", baz: 42 }); | ||
}); | ||
|
||
it("overrides an array with another array", () => { | ||
const first = ["foo", "bar"]; | ||
const second = ["baz", "qux"]; | ||
const result = merge(first, second); | ||
|
||
assert.strictEqual(result, second); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that these cases are not actually supported in eslintrc or in flat config, because arrays are never merged directly. So I've replaced them with similar unit tests below where arrays are wrapped in a property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! I think that we should fix all of the eslintrc incompatibilities in this PR. It was definitely my intent to mirror eslintrc merging behavior. 👍
...first, | ||
...second | ||
}; | ||
|
||
delete result.__proto__; // eslint-disable-line no-proto -- don't merge own property "__proto__" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't want the __proto__
, then I'd suggest using Object.create(null)
to create result
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent here was just to exclude __proto__
when it's an own property, because the inherited property Object.prototype.__proto__
should not be masked or accidentally overwritten (delete
only removes own properties). By contrast, Object.create(null)
creates an object with null
prototype, that lacks all methods of Object.prototype
but could still have an own property called __proto__
, so it's not really the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, so this line is deleting an own property that might have been added through the spread operations. That makes sense to me. 👍
it("merges two arrays in a property", () => { | ||
const first = { someProperty: ["foo", "bar", void 0, "baz"] }; | ||
const second = { someProperty: ["qux", void 0, 42] }; | ||
const result = merge(first, second); | ||
|
||
assert.deepStrictEqual(result, { someProperty: ["qux", "bar", 42, "baz"] }); | ||
confirmLegacyMergeResult(first, second, result); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed eslintrc's behavior, but I'm not sure if it was intentional or maybe an unintentional effect of an intent to just deep-clone arrays. When I update mergeWithoutOverwrite
so that arrays overwrite arrays instead of merging, no tests fail either in the eslintrc project or the eslint project.
function mergeWithoutOverwrite(target, source) {
if (!isNonNullObject(source)) {
return;
}
for (const key of Object.keys(source)) {
if (key === "__proto__") {
continue;
}
- if (isNonNullObject(target[key])) {
+ if (isNonNullObject(target[key]) && !Array.isArray(target[key])) {
mergeWithoutOverwrite(target[key], source[key]);
} else if (target[key] === void 0) {
- if (isNonNullObject(source[key])) {
- target[key] = Array.isArray(source[key]) ? [] : {};
+ if (isNonNullObject(source[key]) && !Array.isArray(source[key])) {
+ target[key] = {};
mergeWithoutOverwrite(target[key], source[key]);
} else if (source[key] !== void 0) {
target[key] = source[key];
}
}
}
}
I'd personally rather expect that a generic deep-merging just overwrites arrays, so that this example results in { someProperty: ["qux", void 0, 42] }
. Merging item-by-item makes sense only when it is known that the array is meant to be used in a way that each position in the array has a specific meaning (i.e., like a tuple).
Now when searching through issues and PRs, I found that we mentioned this in #14321 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, now that comment in code makes sense. So, should the same logic also apply when only one of the values is an array, i.e. should an array overwrite a plain object and vice-versa?
deepMerge({ someProperty: ["foo"] }, { someProperty: { bar: "baz" } });
// expected { someProperty: { bar: "baz" } }
// not { someProperty: { 0: "foo", bar: "baz" } }
deepMerge({ someProperty: { 1: "foo" }, { someProperty: ["bar"] } });
// expected { someProperty: ["bar"] } }
// not { someProperty: ["bar", "foo"] }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes. I'd say the new config system is a good opportunity to change the confusing and possibly unintended specifics of eslintrc's deep merging, but would like to hear @nzakas's opinion too.
Merging array properties into objects and vice versa almost never makes sense.
Merging arrays into arrays might make sense in some specific cases, but I believe it's generally expected that arrays overwrite arrays.
So the behavior would be pretty simple to explain. Merging first
with second
results in:
- Deeply merged object if both
first
andsecond
are plain objects (neither is an array). first
ifsecond
isundefined
.second
in all other cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those changes make sense. Let's do it. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This should be working as suggested after 5d435f6.
I've just noticed another edge case: when a property is undefined in both first and second object - but explicitly set in one or both - eslintrc does not set it at all in the result, while deepMerge({ someProperty: void 0 }, { someProperty: void 0 });
// current result: { someProperty: undefined }
// merge result in eslintrc: {} Any advice/preference on how to handle this? |
lib/config/flat-config-schema.js
Outdated
if (Array.isArray(second)) { | ||
result = Object.assign([], result); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can never happen, because second
is always a non-array object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Removed in 6e2da74.
I think it's fine to leave the property with |
I've updated the title since we're not fully replicating eslintrc behavior. |
I'll close-reopen just to double check CI, since we had a few changes on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Leaving open for @nzakas to verify.
One thing that is different with this implementation, compared to eslintrc and the previous implementation, is that they were deep-cloning all objects and arrays, while this keeps the same instances except for objects that are getting merged with other objects in the process. However, I don't think this is important, as we're not modifying any objects or arrays that were passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for all your work on this. 🙏
* fix: replicate eslintrc merge behavior in flat config * do not merge arrays * Remove unused code * test for undefined properties * fix an edge case with non-enumerable properties (cherry picked from commit f182114)
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #17820
Tell us about your environment (
npx eslint --env-info
):Node version: v21.5.0
npm version: v10.2.4
Local ESLint version: v8.56.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 23.1.0
What parser are you using (place an "X" next to just one item)?
[x]
Default (Espree)
[ ]
@typescript-eslint/parser
[ ]
@babel/eslint-parser
[ ]
vue-eslint-parser
[ ]
@angular-eslint/template-parser
[ ]
Other
Please show your full configuration:
No config file.
What did you do? Please include the actual source code causing the issue.
I compared the merge behavior in flat config against eslintrc in different cases.
What did you expect to happen?
The merge behavior in flat config should be the same as in eslintrc.
What actually happened? Please include the actual, raw output from ESLint.
There are some differences in the merge behavior, all addressed in this PR.
Repro: https://stackblitz.com/edit/stackblitz-starters-7ta5nx
What changes did you make? (Give an overview)
Updated the
deepMerge
algorithm in lib/config/flat-config-schema.js to make it more similar to eslintrc'smergeWithoutOverwrite
. I think this was requested in #17820 (comment).The new changes affect the way properties are merged: we could keep all of them or revert some to the current behavior.
null
property values in the second object will no longer result in a runtime error but instead behave like other primitives and overwrite properties of the first object. This is the original bug reported in Bug: (flat config) ESLint crashes when merging anull
value in a config #17820.__proto__
property, the__proto__
property will no longer be merged. In the current merge implementation, the result will contain an own__proto__
property if the first object does.Arrays can merge with arrays and other objects like in eslintrc. The resulting value will be an array or a plain object, depending on the type of the second value. Currently, merging only occurs when the second value is not an array (although the first value may be any object). I'm not 100% sure of this change because of this comment.There is agreement that in the new config system arrays should not merge with other arrays or objects like in eslintrc.Is there anything you'd like reviewers to focus on?
All of the above points.
As a hint, you may find it helpful to play with the repro when not sure about how things work in the current implementation or in eslintrc.